Skip to content

Update sqlite RemoveWorkflowInstances performance - #475

Open
EricHorton wants to merge 18 commits into
cschleiden:mainfrom
nodeselector:main
Open

Update sqlite RemoveWorkflowInstances performance#475
EricHorton wants to merge 18 commits into
cschleiden:mainfrom
nodeselector:main

Conversation

@EricHorton

Copy link
Copy Markdown

Update the sqlite backend RemoveWorkflowInstances to improve performance and fix a couple bugs that prevented correctly deleting all history.

This PR also updates cancellableCommand state transitions to handle a panic caused by a command completing while in the CancelPanding state.

nodeselector and others added 18 commits February 11, 2026 00:09
Add a test that runs a workflow with an activity, removes it via
RemoveWorkflowInstances, and then asserts that GetWorkflowInstanceHistory
returns empty. The existing test only checks that the instance row is
gone (GetWorkflowInstanceState returns ErrInstanceNotFound) but never
verifies that history and attributes rows are also deleted.

This test currently fails: RemoveWorkflowInstances uses 'id' as the
column name in DELETE statements for the history and attributes tables,
but 'id' in those tables is the event UUID, not the instance ID. The
correct column is 'instance_id'. The DELETE silently matches zero rows,
so instance rows are removed but history and attributes leak.
Three bugs fixed in RemoveWorkflowInstances:

1. Column-name mismatch: DELETE statements for the history and
   attributes tables used "id IN (...)" but the "id" column in those
   tables holds event UUIDs, not instance IDs. The correct column is
   "instance_id". The DELETE silently matched zero rows, so instance
   rows were removed but history and attributes rows leaked.

2. Unbounded SELECT: the initial query selected all matching expired
   instances with no LIMIT. With a large backlog this causes the
   activity to exceed ActivityLockTimeout (default 2m), resulting in
   "context deadline exceeded" on every attempt. The expiration
   workflow retries (MaxAttempts: 2), both fail, and the next attempt
   isn't until the next timer cycle — by which time the backlog is
   larger. Auto-expiration silently never completes.

   Add LIMIT bounded by the configured BatchSize (default 100). Each
   activity invocation processes at most one batch. The expiration
   workflow's built-in loop (10 iterations with ContinueAsNew) handles
   incremental draining across multiple invocations.

3. Defer in loop: "defer tx.Rollback()" inside the batch for-loop
   accumulated deferred calls that only executed at function exit.
   With a single bounded transaction there is no loop.

Also adds "completed_at IS NOT NULL" to the WHERE clause to avoid
matching incomplete instances, and adds missing "defer rows.Close()".
Problem:
Three functions had error handling that only checked for sql.ErrNoRows,
silently dropping all other scan errors:

    if err := row.Scan(&state); err != nil {
        if err == sql.ErrNoRows {
            return backend.ErrInstanceNotFound
        }
    }  // Other errors silently ignored, function continues!

This is dangerous because database errors (connection issues, type
mismatches, etc.) would be swallowed, and the function would continue
with zero-value data, potentially causing silent data corruption or
confusing downstream errors.

Affected functions:
- removeWorkflowInstance: state variable stays zero, leading to
  incorrect "instance not finished" errors
- GetWorkflowInstanceState: returns zero state with nil error
- SignalWorkflow: continues with empty executionID

The fix wraps non-ErrNoRows errors and returns them immediately,
following standard Go error handling patterns.

Before: Non-ErrNoRows errors silently ignored, execution continues
After:  All scan errors propagated to caller with context
Problem:
GetStats queries two result sets (workflowRows, activityRows) but
neither has explicit Close() calls. While the transaction's Rollback()
at function end will eventually release resources, this relies on
implicit behavior and delays cleanup.

In Go's database/sql, best practice is to defer Close() immediately
after obtaining rows:

    rows, err := tx.QueryContext(...)
    if err != nil {
        return nil, err
    }
    defer rows.Close()  // Clean up as soon as we're done iterating

This ensures resources are released promptly, even if we return early
due to scan errors during iteration. It also makes the cleanup path
explicit and self-documenting.

The existing code in getPendingEvents, getHistory, and GetFutureEvents
already follows this pattern - this change brings GetStats into
alignment.

Before: Rows closed implicitly when transaction rolls back at function end
After:  Rows closed immediately after iteration completes
Two issues fixed in this commit:

1. GetFutureEvents: Remove premature rows.Err() check

   The original code checked futureEvents.Err() immediately after
   QueryContext(), but rows.Err() is only populated during iteration,
   not after the query. This check was always nil and thus useless.

   The fix moves the Err() check to after the for loop (where it
   belongs) and moves defer Close() before iteration for proper
   cleanup on early return from scan errors.

   Reference: https://pkg.go.dev/database/sql#Rows.Err
   "Err returns the error, if any, that was encountered during iteration."

2. removeFutureEvent: Return nil instead of stale err variable

   The function ended with 'return err' but 'err' was the result of
   the initial QueryContext call - always nil if we reached that point.
   This was a copy-paste artifact; the correct return is 'return nil'.

   Before: return err  // Always nil, misleading
   After:  return nil  // Explicitly correct

Both fixes align with patterns used elsewhere in the codebase
(getPendingEvents, getHistory) and Go database/sql best practices.
Three bugs fixed in RemoveWorkflowInstances:

1. Column-name mismatch: DELETE statements for the history and
   attributes tables used "id IN (...)" but the "id" column in those
   tables holds event UUIDs, not instance IDs. The correct column is
   "instance_id". The DELETE silently matched zero rows, so instance
   rows were removed but history and attributes rows leaked.

2. Unbounded SELECT: the initial query selected all matching expired
   instances with no LIMIT. With a large backlog this causes the
   activity to exceed ActivityLockTimeout (default 2m), resulting in
   "context deadline exceeded" on every attempt. The expiration
   workflow retries (MaxAttempts: 2), both fail, and the next attempt
   isn't until the next timer cycle — by which time the backlog is
   larger. Auto-expiration silently never completes.

   Add LIMIT bounded by the configured BatchSize (default 100). Each
   activity invocation processes at most one batch. The expiration
   workflow's built-in loop (10 iterations with ContinueAsNew) handles
   incremental draining across multiple invocations.

3. Defer in loop: "defer tx.Rollback()" inside the batch for-loop
   accumulated deferred calls that only executed at function exit.
   With a single bounded transaction there is no loop.

Also adds "completed_at IS NOT NULL" to the WHERE clause to avoid
matching incomplete instances, and adds missing "defer rows.Close()".
The expiration workflow previously called RemoveWorkflowInstances once
per 24h timer, removing at most BatchSize (100) instances. With 313k+
expired instances, this would take ~8.5 years to drain.

Changes:
- Backend.RemoveWorkflowInstances now returns (int, error) — the count
  is already computed (len of the SELECT result), zero extra DB calls
- Expiration workflow loops the activity until removed == 0, capped at
  100 iterations (10k instances per timer tick)
- Each activity call gets its own 2m lock timeout window, avoiding the
  death-spiral where one call tries to process the entire backlog

All backend implementations (sqlite, postgres, mysql, redis) and the
mock updated. Existing tests pass with no flakiness.
Set auto_vacuum(full) in the DSN so new databases are created with
auto_vacuum enabled from the start. For existing databases, check the
current auto_vacuum setting and run VACUUM once to convert if needed.

Without this, deleting workflow instances creates freelist pages but
never returns disk space to the OS. On high-throughput deployments this
causes workflows.db to grow monotonically even with working expiration.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reduce batch size from 500 to 100 and add 1s sleep between batches.
On large databases (100G+), continuous DELETE batches starve
GetWorkflowTask/GetActivityTask by holding the SQLite write lock.

The 1s yield between batches allows task processing to acquire the
lock. At 100 rows/batch with 1s sleep, worst case drain rate is
~6000/min — still fast enough to drain backlogs while keeping tasks
flowing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The auto_vacuum=FULL + VACUUM on startup blocks VMC for hours on
large (100G+) databases. VACUUM belongs in a later PR after draining.
sqlite: fix RemoveWorkflowInstances column names, add LIMIT
sqlite: add defer rows.Close() in GetStats
sqlite: fix error handling in events.go
fix: loop expiration activity to drain backlog incrementally
Handle done while in the CancelPending state.
@EricHorton

Copy link
Copy Markdown
Author

@cschleiden, can we get a review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants